Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(eslint): Added an a11y configuration to our configs #148

Merged
merged 2 commits into from
Sep 25, 2023
Merged

feat(eslint): Added an a11y configuration to our configs #148

merged 2 commits into from
Sep 25, 2023

Conversation

alexasselin008
Copy link
Member

I'm working on a first draft of the eslint configuration to detect accessibility problems. The most popular Eslint plugin is eslint-plugin-jsx-a11y and they offer a "recommended" config.

Feel free to pitch in if you have ideas and/or concerns about the following configuration.

Their default configuration seems great, except 3 rules that we will disable for now.

  • jsx-a11y/no-autofocus: There is a really good article that describes the issues with autoFocus and why it should be avoided. However, this issue is with screen readers and not with keyboard navigation. Since, we use autoFocus in a lot of places to improve the user experience, i'll disable this rule
  • jsx-a11y/media-has-caption: This rule ensures that all media elements have a for captions. Once again an issue of Screen readers only and we generally either embed subtitle in the video, or don't have subtitle at all. So i would disable this rule
  • jsx-a11y/label-has-associated-control: This ensures that all labels have an associated control that they are labelling. However, since our current implementation of Orbit automatically assigns label to control inside Fields, this creates a lot of false positive. I would therefore disable this rule

@patricklafrance
Copy link
Member

jsx-a11y/no-autofocus: There is a really good article that describes the issues with autoFocus and why it should be avoided. However, this issue is with screen readers and not with keyboard navigation. Since, we use autoFocus in a lot of places to improve the user experience, i'll disable this rule

I agree that it should be disabled for us. Furthermore, the article doesn't say that autofocus is always bad, only when the form is not the main purpose of the page/modal and there's no context information associated to the form.

jsx-a11y/label-has-associated-control: This ensures that all labels have an associated control that they are labelling. However, since our current implementation of Orbit automatically assigns label to control inside Fields, this creates a lot of false positive. I would therefore disable this rule

Too bad, I would like to shift the label validation from the Orbit components runtime to a linter. Furthermore, not every application use Orbit and those applications are kind of penalized by this decision.

@patricklafrance
Copy link
Member

patricklafrance commented Sep 25, 2023 •
edited
Loading

LGTM.

The only missing thing is to add the config to this list: https://gsoft-inc.github.io/wl-web-configs/eslint/advanced-composition/#available-pieces

@alexasselin008 alexasselin008 merged commit fdae0cd into main Sep 25, 2023
1 check passed
@alexasselin008 alexasselin008 deleted the feat/a11y branch September 25, 2023 13:54
@github-actions github-actions bot mentioned this pull request Sep 25, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patricklafrance patricklafrance patricklafrance approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alexasselin008 @patricklafrance